Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Sanitize unit test and increase # significant digits PC gradients #721

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Sanitize unit test and increase # significant digits PC gradients #721

wants to merge 2 commits into from

Conversation

pultar
Copy link
Contributor

@pultar pultar commented Nov 17, 2022

Thanks for the fix for #683!

I agree with you (#718 (comment)) that a third system would be overkill for the unit tests. I adjusted my PR accordingly:

  • increase the number of significant digits of PC gradients in the output file to 12 to be consistent with output for gradients and energy
  • the unit test for embedding / C API uses the same system as the Fortran unit test and the documentation (water tetramer) for easier comparison
  • I also check in the unit test for C API for energy, and gradients with the same threshold as elsewhere

I also noted that in the test I had included earlier and that is now replaced: charges were not balanced, potentially leading to numerically less stable calculations, which is not desirable in unit tests. This PR fixes this issue.

Signed-off-by: Felix Pultar [email protected]

MtoLStoN
MtoLStoN reviewed Nov 18, 2022
View reviewed changes
Copy link
Member

@MtoLStoN MtoLStoN left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that having consistent test accuracy is a good practice. Maybe it would be good if we also find some way to test the read_pcme subroutine, as the error that was fixed in #720 was introduced there and not caught by any of our tests.

xtb/src/embedding.f90

Lines 88 to 91 in cb55e0e

conv = 1.0_wp ! input is usually in *Bohr*
if (set%pcem_orca) then
conv = aatoau ! except when we try to read ORCA's pc-files
endif

@pultar
Copy link
Contributor Author

pultar commented Nov 18, 2022

Sounds like a great idea! Would you like to add some code? You should be able to edit my PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MtoLStoN MtoLStoN MtoLStoN left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pultar @MtoLStoN